Remember all previously approved configurations and hooks by steinybot · Pull Request #665 · sds/overcommit

steinybot · 2019-09-01T04:55:50Z

Checking out or rebasing through an older, already approved configuration currently fails signature verification because signing a newer version overwrites the only stored approval. Remember every locally approved configuration and hook version, while continuing to reject unseen changes and unapproved combinations of hook code and configuration.

Fixes #300.

Approvals now live in overcommit-signatures under the repository's Git directory. Each file is indexed by the SHA256 of its signature config key and its existing SHA256 signature, and contains both values for inspection. Lookup reads one fixed-size file regardless of history length. History has no cap and does not grow .git/config or slow Git config reads.

This replaces the original PR's Git blob design and addresses the security concerns in its review:

Approvals are local filesystem data outside the Git object database. Fetching attacker-controlled commits, blobs, tags, or other refs cannot grant trust.
Git garbage collection cannot remove approvals.
Revoke one approval by deleting its file; revoke all approvals by deleting overcommit-signatures. No refs, keys, or cryptographic trust infrastructure are required.
Existing Git config signatures migrate on first use without changing their digest format. The legacy key is removed only after its approval is safely written, so deleting history does not resurrect an old approval.
Linked worktrees share history by default. With extensions.worktreeConfig, both history and the existing verification setting retain their per-worktree isolation.
A hook's code and configuration remain signed together. Individually approved components do not approve new combinations.

README documents storage, migration, worktrees, and revocation. Older Overcommit releases do not read the new history and will require re-signing if used after migration.

Validation:

Full suite: 1,785 examples, zero failures; 35 additional examples compared with current main.
RuboCop and all pre-commit checks pass.
Actual checkout and rebase regression tests fail against unmodified main and pass with this change. The rebase test also verifies the original branch remains attached and its changes are retained.
Tests cover configuration and plugin history, unknown changes, code/configuration combinations, legacy configuration and plugin migration, revocation, worktree isolation, a separate Git directory, garbage collection, forged approval blobs delivered through a remote tag, corrupted files, concurrent writers, 1,000 historical signatures, and read/write/config failures.
Rebased onto current main. All 20 GitHub Actions checks pass: the complete Linux/Windows matrix on Ruby 3.3, 3.4, and 4.0, lint, and the combined CI/coverage job.

sds

Thanks for the pull request with tests, @steinybot.

Some questions:

What does using Git's blob storage ultimately get you?
Will these blobs always remain local? (i.e. can they be pushed to the remote? If they could, that would be bad as an attacker could push fake verified blobs that are automatically accepted by others)
When are unused blobs garbage collected?
How would one "unverify" a configuration with this scheme?

In short, it would be great to understand why we're using blob storage when a file under the .git directory would suffice and not raise the above questions. This justification should be included in the PR description.

See #505 for the previous attempt at this feature. Excited for us to proceed if we can have satisfactory answers to the above questions.

sds · 2019-09-07T18:06:46Z

+      #
+      # @param arg [Hash]
+      # @return [String]
+      def sign_signatures(arg)


Ditto on comment about arg name.

sds · 2019-09-07T18:07:16Z

+      end
+
+      # Returns the object signature for some blob.
+      #


Missing doc for the argument type.

sds · 2019-09-07T18:32:43Z

+      #
+      # @param arg [String]
+      # @return [String]
+      def sign(arg)


Could there be a more descriptive name than the generic term arg?

steinybot · 2019-09-10T22:10:23Z

Thanks for the pull request with tests, @steinybot.

Some questions:

What does using Git's blob storage ultimately get you?

The reason was to solve the "quick lookup" requirement since the time taken to lookup a git object is same as retrieving a file. That depends on the file system but in the worst case it is no worse than how well git itself is performing.

Having it as git objects also makes it possible to audit signatures, although in its current state this doesn't work very well as there needs to be a tree object to group them all together.

If we do it right (i.e. using a tree) then there would be a trivial way of seeing all the overcommit hooks and configuration without any other files and also do diffs between them without including changes from the rest of the repository.

Git is going to move to SHA-256 at some point so this would simplify things.

I also wasn't really a big fan of putting non-git stuff in the .git directory but this isn't really a big deal so long as it is namespaced in a way that makes it obvious that it is for overcommit and avoids collisions. This is not uncommon. Sourcetree creates a .git/sourcetreeconfig file and GitHub creates .git/pull/ID/head for PRs (which only exist on the server unless you fetch them explicitly).

Will these blobs always remain local? (i.e. can they be pushed to the remote? If they could, that would be bad as an attacker could push fake verified blobs that are automatically accepted by others)

From a normal users perspective yes they would be local since there wouldn't be a refspec that points to them so they wouldn't be pushed BUT now I realise that a malicious attacker could intentionally make these signature objects reachable from a refspec (i.e. branch or tag) and then a fetch would retrieve them. This is a serious problem.

When are unused blobs garbage collected?

That's something I hadn't considered. Having unreachable objects is not a good idea.

How would one "unverify" a configuration with this scheme?

With the current implementation then it would be to run git prune.

In short, it would be great to understand why we're using blob storage when a file under the .git directory would suffice and not raise the above questions. This justification should be included in the PR description.

See #505 for the previous attempt at this feature. Excited for us to proceed if we can have satisfactory answers to the above questions.

Clearly there are some things that I regrettably overlooked. The current problems are:

Having the trust stored only in objects is flawed since it is not possible to prevent an attacker from creating arbitrary objects.
Objects must be reachable otherwise gc will remove them.

To solve 1 I think we should be using proper digital signatures rather than just hashes, something like Ed25519. With this we would generate a private key and store that in ~/.overcommit/keys/ and have the path to that in the local git config. The key could also be password protected and we just prompt for it when signing. The signatures themselves can be public or not, it no longer matters.

Having public signatures means that it wouldn't be a big step to start using something like PGP to create a web of trust. In my opinion having everyone on a team being responsible for reviewing every single config change is a bit of a nightmare and usually just results in everyone signing it without checking.

We could add a new ref like how git-notes work, something like .git/refs/overcommit/signatures pointing to a tree. Each entry in that tree is a blob with the name that is the hash of the tree and the contents is the signature of the whole tree.

This doesn't quite solve 2 but that could also just be a new ref under .git/refs/overcommit/configurations which points to a tree where each other tree is a subtree.

The downside of using the refs in this way is that the tree grows as the number of signatures grow. I'm not sure if git notes has this problem but it would seem like it would. We could always limit the branching factor on the tree if it becomes a problem.

Thoughts?

sds · 2019-09-20T12:52:37Z

Thanks for your reply. Apologies for the delay in response, as I am traveling.

If I simplify what you're proposing down to to its core elements, it appears you are asking:

Whether to use public key-based signatures to utilize a trust model similar to PGP
Whether to use a different ref space to store signatures (such as the one used by git-notes)

These still seem more complicated than a simple file of SHA256 signatures, for benefits that don't seem to have strong justification.

With respect to 1: a web of trust model adds complexity and is still considered broken.

With respect to 2: I've never used git-notes, but this seems like we are tying our implementation to another Git feature, rather than simply taking a hash of a configuration and storing that in a file. Furthermore, if it's in a ref space, could an attacker inject notes? Can you trust notes?

My personal experience running this project is that additional complexity needs to have strong reasoning in order to be included. Revoking a signed configuration should be simple (delete a line). Revoking all signed configurations should be simple (delete a file).

If I've mischaracterized these suggestions, please correct me. At this point in time, I am still strongly in favor of someone taking #505 to completion.

Replace Git object storage with SHA256-indexed local approval files. Migrate legacy signatures, preserve worktree scoping, and support simple revocation without growing Git config. Cover checkout and rebase regressions, forged remote objects, migration, garbage collection, concurrency, and error handling.

Copilot

🟡 Changes recommended

Opening an existing approval in write mode truncates it before replacement, allowing concurrent hooks to reject approved configurations; use a same-directory temporary file and atomic rename.

1 open issue

Avoid truncating approvals before atomically replacing them · New

What changed in this PR

This PR addresses #300 by retaining all locally approved configuration and hook signatures, allowing checkout and rebase through approved revisions without re-signing.

Changes:

Adds filesystem-backed approval history outside Git’s object database, with legacy migration and worktree-aware storage.
Updates configuration and plugin verification to recognize previously approved signatures while rejecting unseen combinations.
Documents approval management and adds regression, security, and storage tests.

File	Description
`spec/overcommit/signature_spec.rb`	Tests history storage, migration, revocation, isolation, and failures.
`spec/overcommit/hook_signer_spec.rb`	Tests recognition of previously approved hook versions.
`spec/integration/worktree_signing_spec.rb`	Tests shared and isolated worktree approvals.
`spec/integration/signature_history_spec.rb`	Adds checkout, rebase, migration, and trust-boundary regressions.
`README.md`	Documents storage, migration, worktrees, and revocation.
`lib/overcommit/signature.rb`	Implements local approval history and legacy migration.
`lib/overcommit/hook_signer.rb`	Uses history to verify and record hook approvals.
`lib/overcommit/hook_loader/plugin_hook_loader.rb`	Checks plugins for unapproved signatures.
`lib/overcommit/exceptions.rb`	Adds a signature-history error type.
`lib/overcommit/configuration.rb`	Records and verifies configuration approvals through history.
`lib/overcommit/configuration_loader.rb`	Uses history-based configuration verification.
`lib/overcommit.rb`	Loads the new signature store.
`CHANGELOG.md`	Notes persistent approval history.

_{Review effort: Balanced}

+
+    def add(signature)
+      FileUtils.mkdir_p(@directory)
+      File.write(path(signature), contents(signature), mode: 'w', perm: 0o600)


* origin/main: (108 commits) Remember all previously approved configurations and hooks (sds#665) Keep deleted and renamed paths absent while stashing changes (sds#928) Document the case-sensitive KtLint hook name (sds#927) Update CI actions and pin Ubuntu to clear annotations (sds#926) Preserve standard input bytes when forwarding hooks (sds#925) Preserve Git-quoted filenames when running all hooks (sds#924) Fail when Git cannot enumerate tracked files (sds#923) Add note about disabling default hooks (sds#922) Close files inspected by LineEndings (sds#921) Use Script Host before PowerShell for Windows parent query fallback (sds#920) Cache Windows dependency resolution with daily refreshes (sds#919) Avoid PowerShell startup for Windows parent command queries (sds#918) Avoid repeated CLI startup when preparing integration fixtures (sds#917) Allocate more CI runners to Windows with asymmetric sharding (sds#916) Shard CI tests across runners and add one required CI check (sds#915) Remove EOL Ruby versions from the CI matrix (sds#914) Cut CI test time by 68% while preserving the full test matrix (sds#913) Cut version 0.75.0 (sds#912) Re-enable the full Windows test matrix and fix platform failures (sds#911) Read .overcommit.yml as UTF-8 in the hook template (sds#910) Update Overcommit logo (sds#907) Cut version 0.74.0 (sds#906) Keep each worktree's signatures separate (sds#903) Recognize amend aliases on macOS (sds#904) Exclude files deleted from the working tree from all_files (sds#905) Restore the Ruby 2.6-3.1 test checks (sds#902) Delete subprocess output tempfiles once they are read (sds#901) Stop wrapping commands in cmd.exe on Windows (sds#898) Allow URL-only lines to exceed TextWidth body limit (sds#900) Cut version 0.73.0 (sds#896) Add automated release flow (sds#895) Instruct oxfmt not to fail if no files are matched (sds#894) Add filetypes to oxc tools (sds#893) Add pre commit hook for herb-lint (sds#892) Cut version 0.72.0 (sds#891) Remove maintainability badge (sds#890) Require patched REXML version (sds#889) Fix operation state restoration in linked worktrees (sds#887) Fail Flay hook when flay crashes instead of silently passing (sds#885) Fix Solargraph regexp capture warning (sds#886) Cut version 0.71.0 (sds#883) Prepare for the new solargraph typecheck format (sds#873) Make hook context input reads thread-safe (sds#882) Cut version 0.70.0 (sds#880) Add oxc pre-commit hooks (sds#879) Add Ruby 3.4 and 4.0 to CI matrix (sds#878) Cut version 0.69.0 (sds#877) Use Etc.nprocessors and handle sandboxed environments (sds#876) Cut version 0.68.0 (sds#872) Add solargraph pre-commit hook for typechecking (sds#871) Update RuboCop in Gemfile to silence a deprecation warning in build (sds#870) Cut version 0.67.1 adds 'set' requirements to diff hook context. (sds#866) Add note about unsupported YAML features in `gemfile:` line in default.yml (sds#865) Cut version 0.67.0 (sds#864) Restore ability to specify "gemfile: false" in config (sds#863) Cut version 0.66.0 (sds#861) Add `-diff` cli option for running precommit hooks against diffs (sds#860) Cut version 0.65.0 Load bundled gems on expected version (sds#859) Update changelog Bump rexml to >= 3.3.9 to resolve GHSA-2rxp-v6pw-ch6m (sds#857) Expand tildes(~) for hooksPath (sds#853) Fix spec compatibility with recent git versions (sds#854) Update changelog Cut version 0.64.0 (sds#852) Add changelog_uri to gemspec (sds#851) Support stylelint 16+, which emits messages on stderr (sds#848) Add Ruby 3.3 to CI matrix (sds#849) Update README with latest requirements (sds#845) Fix README badges (sds#844) Cut version 0.63.0 (sds#843) Add RSpec Hook to PreCommit, Add Applicable files if set (sds#842) Fix RuboCop warnings (sds#841) [FEATURE] New pre-commit hook: Sorbet (Issue sds#825) (sds#826) Green CI PoC (sds#833) Revert "Get suite running smoothly again (sds#824)" (sds#840) Get suite running smoothly again (sds#824) Cut version 0.62.0 (sds#821) Allow childprocess 5 (sds#820) Update branch for CI from `master` to `main` (sds#819) Update RuboCop (sds#818) Adding Ruby 3.2 to test matrix (sds#816) Cut version 0.61.0 (sds#817) fix: local overcommit file merges with existing .overcommit.yml file (sds#815) Allow ReplaceBranch to use skip_if (sds#809) Cut version 0.60.0 (sds#807) Add non ASCII encoding schema support to RailsSchemaUpToDate hook (sds#786) Rescue Psych::DisallowedClass in YamlSyntax (sds#805) Add optional additional local configuration (sds#799) Add MixTest hook (sds#802) Add MixFormat hook (sds#801) support trailing space in replace_branch's 'replacement_text' config (sds#804) Allow overriding Gemfile.lock (sds#796) Cut version 0.59.1 (sds#785) Revert addition of `--disable-pending-cops` flag to RuboCop (sds#783) Revert `Fix encoding of process output under windows` (sds#784) Cut version 0.59.0 (sds#780) Partial log flushing to make long hooks intuitive (sds#777) Fix pronto hooks swallowing generic errors (sds#779) ... # Conflicts: # CHANGELOG.md # README.md

sds reviewed Sep 7, 2019

View reviewed changes

steinybot and others added 8 commits November 9, 2026 22:11

Store signatures as git blobs

289f66c

Store hook signatures as git blobs

7f520dd

Store hook signatures as git blobs

672d13c

Fix git config arguments

67e8b53

Fix verify signatures result

b2a8623

Configure RSpec example status persistence file

2d67a62

Fix hook signer tests

724ea09

sds force-pushed the 300-remember-all-previously-verified-configurations branch from 15dd91d to acb75aa Compare November 10, 2026 06:18

Copilot AI balanced review requested due to automatic review settings November 10, 2026 06:19

sds changed the title ~~300 remember all previously verified configurations~~ Remember all previously approved configurations and hooks Nov 10, 2026

Copilot started reviewing on behalf of steinybot November 10, 2026 06:19

View session

sds merged commit d788ab8 into sds:main Nov 10, 2026
20 checks passed

Copilot AI reviewed Nov 10, 2026

View reviewed changes

Comment thread lib/overcommit/signature.rb

def add(signature)

FileUtils.mkdir_p(@directory)

File.write(path(signature), contents(signature), mode: 'w', perm: 0o600)

Navigation Menu

Repository navigation

Remember all previously approved configurations and hooks - #665

Remember all previously approved configurations and hooks#665
sds merged 8 commits into
sds:mainfrom
steinybot:300-remember-all-previously-verified-configurations

steinybot commented Sep 1, 2019 •

edited by sds

Loading

Uh oh!

sds left a comment

Uh oh!

sds Sep 7, 2019

Uh oh!

sds Sep 7, 2019

Uh oh!

sds Sep 7, 2019

Uh oh!

steinybot commented Sep 10, 2019

Uh oh!

sds commented Sep 20, 2019

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

steinybot commented Sep 1, 2019 • edited by sds Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sds left a comment

Choose a reason for hiding this comment

Uh oh!

sds Sep 7, 2019

Choose a reason for hiding this comment

Uh oh!

sds Sep 7, 2019

Choose a reason for hiding this comment

Uh oh!

sds Sep 7, 2019

Choose a reason for hiding this comment

Uh oh!

steinybot commented Sep 10, 2019

Uh oh!

sds commented Sep 20, 2019

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steinybot commented Sep 1, 2019 •

edited by sds

Loading